-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option to forward laz header info #149
Conversation
I've always wondered if we should be doing that when we write LAS files. The spec is ambiguous about it ASPRSorg/LAS#138 |
Do we need this to be an option? What's the downside of always doing this? |
None as far as I'm concerned. Maybe it is best to always forward in the single input case. Note that I'll be adding support for setting additional VLRs and explicitly setting more LAS header fields in the coming months. |
Should I interpret the above as:
? |
Fine with me, but it is up to you.
I say leave it as you currently have implemented – if nothing to forward, use |
@@ -63,10 +67,18 @@ struct BaseInfo | |||
DimInfoList dimInfo; | |||
pdal::SpatialReference srs; | |||
int pointFormatId; | |||
uint16_t globalEncoding {0}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given what we're doing here, would it make sense just to include an entire lazperf::header, rather than all these separate fields? I think that would make it more clear what this data is all about, rather than just have it be random stuff sitting in the BaseInfo block. We can merge this and I can take care of that change if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that too when I kept adding members there, but wasn't too sure, as there will be unused lazperf::header(14?)
variables there that might be confusing.
bu/CopcSupport.cpp
Outdated
@@ -42,11 +42,15 @@ CopcSupport::CopcSupport(const BaseInfo& b) : m_b(b), | |||
m_f.open(toNative(b.opts.outputName), std::ios::out | std::ios::binary); | |||
|
|||
//ABELL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These "//ABELL" comments should be removed. There were just there to note that the values were crap, but you've fixed that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
untwine/Common.hpp
Outdated
@@ -47,6 +48,7 @@ struct Options | |||
bool stats; | |||
std::string a_srs; | |||
bool metadata; | |||
bool forwardAll; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote for removing the option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
Fix stringop-truncation warning
This PR adds a
--forward_all
option. When this option is used in single file mode, and a single input las/laz file is used then the following header info are preserved:This will ultimately be used in QGIS by default so that Untwine is a transparent stage for the end user.
Also, when the
--forward_all
option is not used, the creation day/year is now correctly populated and the generating software is populated withUntwine
Fixes #139